Framework: Capture and recover from application error #3208

aduth · 2017-10-27T16:19:03Z

Related: #2410

This pull request seeks to add application-wide error handling to try to prevent content loss in the case that an unhandled error occurs in editor rendering. This is intended as a last-resort recovery to avoid a "blank screen" case, and favors state integrity over presentation, in order to avoid any risk that an error occurs during the recovery itself. There could be improvements made here to try to alleviate any stress to a user who finds themselves in this scenario.

As implemented, the user is provided options to attempt a recovery or copy the raw state data to their clipboard. In the former case, the behavior is to attempt a re-render, providing as initial state to the Redux store the state which had existed at the time of the crash. It is possible that this recovery may not be possible, if it is the state itself which is the cause of the error. Therefore, in order to provide some means of recovery, the user is also presented the option to retrieve the underlying state data from which they may be able to extract remnants of their post state.

Video Demonstration

Testing instructions:

Ideally there is no way to trigger an application error. You may introduce one though, ideally which doesn't occur at initial render. I have been using the following to trigger an error when a dropdown is opened (e.g. inserter menu):

diff --git a/components/dropdown/index.js b/components/dropdown/index.js
index 1989f101..1c79f148 100644
--- a/components/dropdown/index.js
+++ b/components/dropdown/index.js
@@ -61,6 +61,7 @@ class Dropdown extends Component {
 
        render() {
                const { isOpen } = this.state;
+               if ( isOpen ) throw new Error();
                const { renderContent, renderToggle, position = 'bottom', className, contentClassName } = this.props;
                const args = { isOpen, onToggle: this.toggle, onClose: this.close };
                return (

When an application error occurs, note that you are presented with a dialog with options to either attempt recovery or copy to state. Attempt recovery should restore the editor to its state prior to the error occurring. Copying state should copy a (very large) serialized state object to your clipboard.

youknowriad

Works pretty well.

I'm wondering about metaboxes, what happens when they raise in error. Should we have a special ErrorBoundary for metaboxes? Especially in this PR #3345

youknowriad · 2017-11-08T08:12:27Z

components/clipboard-button/index.js

 	}

 	render() {
-		const { className, children } = this.props;
+		// Disable reason: Exclude from spread props passed to Button


I usually use omit for this. I can follow this approach for consistency though

The ESLint warning is not great, and we've had to disable it elsewhere. I just found this today, and am inclined to enable the option for our configuration:

https://eslint.org/docs/rules/no-unused-vars#ignorerestsiblings

omit is fine too, but means an extra dependency and also duplicating the references (once in destructure, once in omit keys listing).

I guess it wouldn't be a verbatim duplication, since text here is in-fact unused. I see some value in leaving ignoreRestSIblings as the default false behavior, but... it is a convenient omitting technique. Hmm!

youknowriad · 2017-11-08T08:14:12Z

editor/error-boundary/index.js

+					<Button onClick={ this.reboot } isLarge>
+						{ __( 'Attempt Recovery' ) }
+					</Button>
+					<ClipboardButton text={ this.getStateText } isLarge>


In addition to the state, is there a way to provide the stack trace. People could send us both to debug quickly.

Yeah, that makes sense. Maybe they can be combined into a single "Copy Debug Info" button?

Sounds good to me

youknowriad · 2017-11-08T08:16:02Z

editor/store.js

 * @return {Redux.Store} Redux store
 */
-function createReduxStore() {
+function createReduxStore( initialState ) {
 	const enhancers = [
 		applyMiddleware( multi, refx( effects ) ),
 		storePersist( 'preferences', GUTENBERG_PREFERENCES_KEY ),


What happens to the preferences when we use an "initialState" should we ignore hydrating them?

gziolo · 2017-11-08T08:35:15Z

editor/store.js

@@ -33,7 +34,7 @@ function createReduxStore() {
 		enhancers.push( window.__REDUX_DEVTOOLS_EXTENSION__() );
 	}

-	const store = createStore( reducer, flowRight( enhancers ) );
+	const store = createStore( reducer, initialState, flowRight( enhancers ) );


Since we are updating this line, can we inline this const? It seems obsolete.

gziolo · 2017-11-08T08:37:36Z

editor/warning/index.js

+ */
+import './style.scss';
+
+function Warning( { children } ) {


Good idea to move it up and make it a general purpose component 👍

gziolo · 2017-11-08T08:42:36Z

editor/store.js

 * @return {Redux.Store} Redux store
 */
-function createReduxStore() {
+function createReduxStore( initialState ) {


Would it make sense to rename it to preloadedState to match with Redux API? See: https://github.com/reactjs/redux/blob/e58e207213161042cb5853ddb6acb0d65034c3b0/src/createStore.js#L39.

gziolo · 2017-11-08T08:56:42Z

editor/index.js

+		>
+			<ErrorBoundary onError={ reboot }>
+				<Layout />
+			</ErrorBoundary>


When createEditorInstance is executed on the initial page load we also run initializeMetaBoxes. I think we don't need to do it again when rebooting, but wanted to double check.

When createEditorInstance is executed on the initial page load we also run initializeMetaBoxes. I think we don't need to do it again when rebooting, but wanted to double check.

Correct, I would expect we shouldn't need to re-dispatch the action, since the effects of the original initialization should be reflected in the state that is used for rebooting.

gziolo · 2017-11-08T08:57:47Z

editor/index.js

 	const target = document.getElementById( id );

+	// When an unhandled error occurs, user can choose to reboot the editor,
+	// replacing a previous mount using initialState from the crashed state.
+	unmountComponentAtNode( target );


I noticed that we don't need to call unmountComponentAtNode when no initialState is passed. Not sure if we really need to change that.

I noticed that we don't need to call unmountComponentAtNode when no initialState is passed. Not sure if we really need to change that.

Yeah, it doesn't harm anything to keep it this way, but could make things clearer if the condition were to demonstrate it only occurs when rebooting.

gziolo · 2017-11-08T09:07:32Z

editor/index.js

 * @return {Object} Editor interface. Currently supports metabox initialization.
 */
-export function createEditorInstance( id, post, settings ) {
+export function createEditorInstance( id, post, settings, initialState ) {


It's a bit confusing for me when I look at it for the first time. At the moment we pass post which gets later injected into Redux state, settings which are passed directly to the EditorProvider and then optional initialState which again operates on Redux state. My question is if it would make sense to provide a different handler for rebooting. It will increase the number of lines and introduce some code duplication, but it might improve readability here. What do you think?

gziolo

This PR looks like a great improvement and solves a real problem. I think I encountered this issue a few times already. I left some comments, but they aren't blockers. I was trying to better understand how Redux store works in Gutenberg, so I left some questions where I wasn't sure how things work. I also added some notes where I think we might consider refactoring the existing code.

aduth · 2017-11-08T21:40:42Z

I'm wondering about metaboxes, what happens when they raise in error. Should we have a special ErrorBoundary for metaboxes?

I think that's reasonable. This is definitely meant as a last-resort measure, and we should be more granular (and ideally more user-friendly) with error boundaries further down in the tree (block error boundaries being a good example).

gziolo · 2017-11-09T09:47:27Z

I was thinking about something along those lines to make createEditorInstance independent from reboot:

diff --git a/editor/index.js b/editor/index.js
index 937e7b35..8836b954 100644
--- a/editor/index.js
+++ b/editor/index.js
@@ -45,6 +45,31 @@ window.jQuery( document ).on( 'heartbeat-tick', ( event, response ) => {
 } );
 
 /**
+ * When an unhandled error occurs, user can choose to reboot the editor,
+ * replacing a previous mount using initialState from the crashed state.
+ *
+ * @param {Element} target       Target element where editor gets recreated
+ * @param {?Object} settings     Editor settings object
+ * @param {?Object} initialState Initial editor state to hydrate
+ */
+function recreateEditorInstance( target, settings, initialState ) {
+	unmountComponentAtNode( target );
+	const reboot = recreateEditorInstance.bind( null, target, settings );
+
+	render(
+		<EditorProvider
+			settings={ settings }
+			initialState={ initialState }
+		>
+			<ErrorBoundary onError={ reboot }>
+				<Layout />
+			</ErrorBoundary>
+		</EditorProvider>,
+		target
+	);
+}
+
+/**
  * Initializes and returns an instance of Editor.
  *
  * The return value of this function is not necessary if we change where we
@@ -53,22 +78,16 @@ window.jQuery( document ).on( 'heartbeat-tick', ( event, response ) => {
  * @param {String}  id           Unique identifier for editor instance
  * @param {Object}  post         API entity for post to edit
  * @param {?Object} settings     Editor settings object
- * @param {?Object} initialState Initial editor state to hydrate
  * @return {Object} Editor interface. Currently supports metabox initialization.
  */
-export function createEditorInstance( id, post, settings, initialState ) {
+export function createEditorInstance( id, post, settings ) {
 	const target = document.getElementById( id );
-
-	// When an unhandled error occurs, user can choose to reboot the editor,
-	// replacing a previous mount using initialState from the crashed state.
-	unmountComponentAtNode( target );
-	const reboot = createEditorInstance.bind( null, id, post, settings );
+	const reboot = recreateEditorInstance.bind( null, target, settings );
 
 	const provider = render(
 		<EditorProvider
 			settings={ settings }
 			post={ post }
-			initialState={ initialState }
 		>
 			<ErrorBoundary onError={ reboot }>
 				<Layout />
diff --git a/editor/provider/index.js b/editor/provider/index.js
index 75262812..e85dfec5 100644
--- a/editor/provider/index.js
+++ b/editor/provider/index.js
@@ -45,9 +45,7 @@ class EditorProvider extends Component {
 
 		const store = createReduxStore( props.initialState );
 
-		// If initial state is passed, assume that we don't need to initialize,
-		// as in the case of an error recovery.
-		if ( ! props.initialState ) {
+		if ( props.post ) {
 			store.dispatch( setupEditor( props.post ) );
 		}

ephox-mogran · 2017-11-13T03:34:12Z

This seems like a really good idea. We've hit quite a few cases recently where the screen has just gone blank and we've suffered possible content loss.

youknowriad · 2017-11-13T08:14:30Z

@EphoxJames Might be a good idea to provide a way to copy the HTML content when it happens.

BoardJames · 2017-11-13T23:37:42Z

@youknowriad I will pass that on to @ephox-mogran 😃

codecov · 2017-11-14T15:24:01Z

Codecov Report

Merging #3208 into master will decrease coverage by 0.06%.
The diff coverage is 6.89%.

@@            Coverage Diff             @@
##           master    #3208      +/-   ##
==========================================
- Coverage   34.22%   34.16%   -0.07%     
==========================================
  Files         257      258       +1     
  Lines        6740     6755      +15     
  Branches     1223     1224       +1     
==========================================
+ Hits         2307     2308       +1     
- Misses       3741     3754      +13     
- Partials      692      693       +1

Impacted Files	Coverage Δ
element/index.js	`100% <ø> (ø)`	⬆️
editor/components/warning/index.js	`0% <ø> (ø)`
editor/index.js	`0% <0%> (ø)`	⬆️
editor/components/provider/index.js	`12.5% <0%> (-0.84%)`	⬇️
...ditor/modes/visual-editor/invalid-block-warning.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/block-crash-warning.js	`0% <0%> (ø)`	⬆️
components/clipboard-button/index.js	`0% <0%> (ø)`	⬆️
editor/store.js	`83.33% <100%> (ø)`	⬆️
editor/components/error-boundary/index.js	`7.69% <7.69%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63f546d...cc658ae. Read the comment docs.

aduth · 2017-11-14T15:25:32Z

Rebased to account for folder restructuring of #3389.

Changed button options in c322687 to drop "Copy State", add "Copy Post Text" and "Copy Error".

Changed block warning messaging in cc2da7d for consistency and addressing #3397.

Closes #3397 Consistency with new application-wide error capturing messaging.

gziolo · 2017-11-14T18:10:05Z

editor/index.js

+	const reboot = recreateEditorInstance.bind( null, target );
+
+	render(
+		<EditorProvider initialState={ initialState }>


Can you confirm that we can safely omit „settings” prop here?

Hmm, on second glance, I think this may be needed. I had assumed this was being used to initialize state and therefore would be preserved in the reboot, but in fact this is part of a separate context. Will create a pull request to reintroduce.

aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Oct 27, 2017

aduth mentioned this pull request Nov 6, 2017

Gutenberg whitescreens if you register a block with a syntax error #2701

Closed

aduth force-pushed the add/layout-error-boundary branch from b9fe096 to 7fb2741 Compare November 7, 2017 20:41

aduth requested review from youknowriad and gziolo November 7, 2017 20:41

youknowriad approved these changes Nov 8, 2017

View reviewed changes

gziolo reviewed Nov 8, 2017

View reviewed changes

gziolo approved these changes Nov 8, 2017

View reviewed changes

aduth mentioned this pull request Nov 8, 2017

Adjust error message on blocks to be less scary and more informative #3397

Closed

aduth added 3 commits November 14, 2017 10:19

Components: Enable ClipboardButton to generate text by callback

3a92956

Components: Pass ClipboardButton extra props to Button

9c4f8bb

Components: Render ClipboardButton as span to avoid layout implications

29325a5

aduth force-pushed the add/layout-error-boundary branch from 7fb2741 to cc2da7d Compare November 14, 2017 15:23

aduth added 3 commits November 14, 2017 10:27

Framework: Capture and recover from application error

bcf9e75

Framework: Change copy options to post text, error stack

a1d497e

Block: Reduce suffering of block errors

cc658ae

Closes #3397 Consistency with new application-wide error capturing messaging.

aduth force-pushed the add/layout-error-boundary branch from cc2da7d to cc658ae Compare November 14, 2017 15:28

aduth merged commit e2ce97d into master Nov 14, 2017

aduth deleted the add/layout-error-boundary branch November 14, 2017 15:40

gziolo reviewed Nov 14, 2017

View reviewed changes

aduth mentioned this pull request Nov 15, 2017

Framework: Preserve settings when rebooting from crash #3500

Merged

aduth mentioned this pull request Aug 7, 2018

Unhandled error dialog is unstyled #8677

Closed

mirka mentioned this pull request Jun 22, 2022

Lodash: Remove from Navigation components #41865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Capture and recover from application error #3208

Framework: Capture and recover from application error #3208

aduth commented Oct 27, 2017 •

edited

Loading

youknowriad left a comment

youknowriad Nov 8, 2017

aduth Nov 8, 2017

aduth Nov 8, 2017

youknowriad Nov 8, 2017

aduth Nov 8, 2017

youknowriad Nov 9, 2017

youknowriad Nov 8, 2017

gziolo Nov 8, 2017

gziolo Nov 8, 2017

gziolo Nov 8, 2017

gziolo Nov 8, 2017

aduth Nov 8, 2017

gziolo Nov 8, 2017

aduth Nov 8, 2017

gziolo Nov 8, 2017

gziolo left a comment

aduth commented Nov 8, 2017

gziolo commented Nov 9, 2017

ephox-mogran commented Nov 13, 2017

youknowriad commented Nov 13, 2017

BoardJames commented Nov 13, 2017 •

edited

Loading

codecov bot commented Nov 14, 2017 •

edited

Loading

aduth commented Nov 14, 2017

gziolo Nov 14, 2017

aduth Nov 15, 2017 •

edited

Loading

Framework: Capture and recover from application error #3208

Framework: Capture and recover from application error #3208

Conversation

aduth commented Oct 27, 2017 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Nov 8, 2017

gziolo commented Nov 9, 2017

ephox-mogran commented Nov 13, 2017

youknowriad commented Nov 13, 2017

BoardJames commented Nov 13, 2017 • edited Loading

codecov bot commented Nov 14, 2017 • edited Loading

Codecov Report

aduth commented Nov 14, 2017

Choose a reason for hiding this comment

aduth Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

aduth commented Oct 27, 2017 •

edited

Loading

BoardJames commented Nov 13, 2017 •

edited

Loading

codecov bot commented Nov 14, 2017 •

edited

Loading

aduth Nov 15, 2017 •

edited

Loading